RFC: Introduce `ArtifactDownloads` stdlib #55755

topolarity · 2024-09-12T13:04:24Z

This shamelessly copies out of Pkg all of the bits required to actually download artifacts, etc. The motivation for this stdlib is to reduce the loading / dependency cost for downloading artifacts, esp. via LazyArtifacts.

Draft because:

ArtifactDownloads.jl should live in an external repo
staticdata.c change was done blindly and needs separate review

With this PR (+ LazyArtifacts.patch):

$ ./julia -e '@time using MicroMamba'
  0.150355 seconds (154.71 k allocations: 8.514 MiB)

versus master (945517b):

$ ./julia -e '@time using MicroMamba'
  1.857587 seconds (932.05 k allocations: 56.118 MiB, 6.27% gc time, 13.45% compilation time: 3% of which was recompilation)

Detailed timings

PR (+ LazyArtifacts.patch):

$ ./julia -e 'using InteractiveUtils; @time_imports using MicroMamba'
      0.6 ms  Printf
     51.0 ms  Dates
      0.5 ms  Scratch
      0.8 ms  TOML
     38.7 ms  ArgTools
      1.1 ms  Tar
               ┌ 0.0 ms NetworkOptions.__init__()
      3.0 ms  NetworkOptions
               ┌ 0.1 ms nghttp2_jll.__init__()
      3.8 ms  nghttp2_jll
               ┌ 0.3 ms LibCURL_jll.__init__()
      3.5 ms  LibCURL_jll
               ┌ 0.0 ms MozillaCACerts_jll.__init__()
      4.2 ms  MozillaCACerts_jll
               ┌ 0.0 ms LibCURL.__init__()
      2.4 ms  LibCURL
               ┌ 0.1 ms Downloads.Curl.__init__()
     44.2 ms  Downloads
               ┌ 0.1 ms p7zip_jll.__init__()
      4.9 ms  p7zip_jll
      1.5 ms  ArtifactDownloads
     21.9 ms  LazyArtifacts
     19.2 ms  Preferences
      0.6 ms  JLLWrappers
               ┌ 0.2 ms micromamba_jll.__init__()
      0.7 ms  micromamba_jll
      0.2 ms  MicroMamba

master (945517b):

$ ./julia -e 'using InteractiveUtils; @time_imports using MicroMamba'
      0.8 ms  Printf
     33.2 ms  Dates
      0.5 ms  Scratch
      0.9 ms  TOML
               ┌ 0.0 ms NetworkOptions.__init__()
      4.8 ms  NetworkOptions
               ┌ 0.2 ms MbedTLS_jll.__init__()
      4.7 ms  MbedTLS_jll
               ┌ 0.1 ms LibSSH2_jll.__init__()
      3.9 ms  LibSSH2_jll
               ┌ 0.2 ms LibGit2_jll.__init__()
      5.8 ms  LibGit2_jll
     23.8 ms  LibGit2
     23.3 ms  ArgTools
               ┌ 0.1 ms nghttp2_jll.__init__()
      4.3 ms  nghttp2_jll
               ┌ 0.2 ms LibCURL_jll.__init__()
      4.1 ms  LibCURL_jll
               ┌ 0.0 ms MozillaCACerts_jll.__init__()
      3.4 ms  MozillaCACerts_jll
               ┌ 0.0 ms LibCURL.__init__()
      3.9 ms  LibCURL
               ┌ 0.2 ms Downloads.Curl.__init__()
     51.9 ms  Downloads
      1.3 ms  Tar
               ┌ 0.1 ms p7zip_jll.__init__()
      5.3 ms  p7zip_jll
      0.3 ms  UUIDs
      0.2 ms  Logging
               ┌ 0.0 ms Pkg.__init__()
    865.5 ms  Pkg
      0.3 ms  LazyArtifacts
     25.6 ms  Preferences
      1.1 ms  JLLWrappers
               ┌ 232.7 ms micromamba_jll.__init__() 99.87% compilation time
    234.6 ms  micromamba_jll 99.06% compilation time
      0.4 ms  MicroMamba

@time

This shamelessly copies out of Pkg all of the bits required to actually download artifacts, etc. The motivation for this stdlib is to reduce the loading / dependency cost for downloading artifacts, esp. via LazyArtifacts. With this PR: ```console $ ./julia -e '@time using MicroMamba' 0.179380 seconds (188.20 k allocations: 10.800 MiB) ``` versus master: ```console $ ./julia -e '@time using MicroMamba' 1.857587 seconds (932.05 k allocations: 56.118 MiB, 6.27% gc time, 13.45% compilation time: 3% of which was recompilation) ```

IanButterworth · 2024-09-12T14:57:14Z

Not sure it's a good idea yet, but nice work! 😁

Benchmarking nit.. The master comparison doesn't look like latest master ┌ 232.7 ms micromamba_jll.__init__() 99.87% compilation time?
I get a total of 0.376127 seconds on 945517b

% ./julia --start=no -e 'using InteractiveUtils; @time @time_imports using MicroMamba'
      0.5 ms  Printf
     17.2 ms  Dates
      0.5 ms  Scratch
      0.5 ms  TOML
               ┌ 0.0 ms NetworkOptions.__init__()
      1.2 ms  NetworkOptions
               ┌ 0.9 ms MbedTLS_jll.__init__()
      2.7 ms  MbedTLS_jll
               ┌ 0.4 ms LibSSH2_jll.__init__()
      2.1 ms  LibSSH2_jll
               ┌ 0.7 ms LibGit2_jll.__init__()
      2.4 ms  LibGit2_jll
     10.8 ms  LibGit2
      5.9 ms  ArgTools
               ┌ 0.3 ms nghttp2_jll.__init__()
      2.5 ms  nghttp2_jll
               ┌ 0.7 ms LibCURL_jll.__init__()
      2.7 ms  LibCURL_jll
               ┌ 0.0 ms MozillaCACerts_jll.__init__()
      1.8 ms  MozillaCACerts_jll
               ┌ 0.0 ms LibCURL.__init__()
      1.2 ms  LibCURL
               ┌ 2.2 ms Downloads.Curl.__init__()
     16.0 ms  Downloads
      1.0 ms  Tar
               ┌ 0.0 ms p7zip_jll.__init__()
      2.4 ms  p7zip_jll
      0.4 ms  UUIDs
      0.2 ms  Logging
               ┌ 0.0 ms Pkg.__init__()
    176.2 ms  Pkg
      0.4 ms  LazyArtifacts
      6.4 ms  Preferences
      0.4 ms  JLLWrappers
               ┌ 86.0 ms micromamba_jll.__init__() 99.81% compilation time
     86.6 ms  micromamba_jll 99.18% compilation time
      0.5 ms  MicroMamba
  0.376127 seconds (759.80 k allocations: 46.579 MiB, 12.69% gc time, 23.29% compilation time: 2% of which was recompilation)

topolarity · 2024-09-12T15:23:27Z

The master results are from 945517b - I'm just running on a totally different set of hardware than you (this was a quick measurement taken from my laptop)

IanButterworth · 2024-09-12T15:27:45Z

Fair enough. Pkg is so slow for you 865.5 ms Pkg

Again shamelessly ripped from Pkg - this only covers part of the functionality moved here (PlatformEngines). Moving the `Pkg.Artifacts` tests will be more involved, since they were written to also test interaction with a lot of other Pkg functionality.

fingolfin

Thank you! Sounds like a good idea to me. I see no reason why downloading artifacts should require Pkg.

fingolfin · 2024-09-19T23:18:29Z

src/staticdata.c

@@ -890,7 +890,7 @@ static void jl_insert_into_serialization_queue(jl_serializer_state *s, jl_value_
    }

    if (immediate) // must be things that can be recursively handled, and valid as type parameters
-        assert(jl_is_immutable(t) || jl_is_typevar(v) || jl_is_symbol(v) || jl_is_svec(v));
+        assert(jl_is_immutable(t) || jl_is_typevar(v) || jl_is_symbol(v) || jl_is_svec(v) || jl_is_module(v));


Why is this needed?

Without it, adding this precompile statement to LazyArtifacts causes this assertion to fail:

precompile(Tuple{typeof(Artifacts._artifact_str), Module, String, Base.SubString{String}, String, Base.Dict{String, Any}, Base.SHA1, Base.BinaryPlatforms.Platform, Val{LazyArtifacts}})

since this ends up serializing a Module as a type parameter

That's valid in principle, but it's still a TODO item here to make sure the serializer handles this correctly.

fingolfin · 2024-10-25T11:59:01Z

So I guess this mainly needs some more comments / buy-in from maybe some core people before its worthwhile to pursue turning it into a full-fledged proposal?

@IanButterworth already commented (thanks), would be good to hear what others like e.g. @KristofferC @staticfloat think

staticfloat · 2024-10-25T20:36:34Z

I agree with this in principle. All that remains are implementation details.

fingolfin · 2024-12-15T00:46:58Z

@topolarity if you are still interested in getting this over the finish line, that'd be awesome

topolarity requested review from KristofferC and IanButterworth September 12, 2024 13:04

topolarity mentioned this pull request Sep 12, 2024

avoid loading pkg until needed JuliaPackaging/LazyArtifacts.jl#1

Open

topolarity added 2 commits September 12, 2024 12:30

rm LibGit2 dependency

1f1d784

Add a few basic tests

43eeed4

Again shamelessly ripped from Pkg - this only covers part of the functionality moved here (PlatformEngines). Moving the `Pkg.Artifacts` tests will be more involved, since they were written to also test interaction with a lot of other Pkg functionality.

fingolfin reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Introduce `ArtifactDownloads` stdlib #55755

RFC: Introduce `ArtifactDownloads` stdlib #55755

topolarity commented Sep 12, 2024 •

edited

Loading

IanButterworth commented Sep 12, 2024

topolarity commented Sep 12, 2024

IanButterworth commented Sep 12, 2024

fingolfin left a comment

fingolfin Sep 19, 2024

topolarity Sep 20, 2024 •

edited

Loading

fingolfin commented Oct 25, 2024

staticfloat commented Oct 25, 2024

fingolfin commented Dec 15, 2024

RFC: Introduce ArtifactDownloads stdlib #55755

Are you sure you want to change the base?

RFC: Introduce ArtifactDownloads stdlib #55755

Conversation

topolarity commented Sep 12, 2024 • edited Loading

Detailed timings

IanButterworth commented Sep 12, 2024

topolarity commented Sep 12, 2024

IanButterworth commented Sep 12, 2024

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Sep 19, 2024

Choose a reason for hiding this comment

topolarity Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

fingolfin commented Oct 25, 2024

staticfloat commented Oct 25, 2024

fingolfin commented Dec 15, 2024

RFC: Introduce `ArtifactDownloads` stdlib #55755

RFC: Introduce `ArtifactDownloads` stdlib #55755

topolarity commented Sep 12, 2024 •

edited

Loading

topolarity Sep 20, 2024 •

edited

Loading